-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement lazy BIND
#1543
Implement lazy BIND
#1543
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1543 +/- ##
==========================================
+ Coverage 88.33% 88.45% +0.12%
==========================================
Files 362 362
Lines 27319 27455 +136
Branches 3682 3705 +23
==========================================
+ Hits 24131 24286 +155
+ Misses 1952 1938 -14
+ Partials 1236 1231 -5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick implementation, I have only some relatively minor questions and requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am deeply sorry, I had already added some comments last week, but never submitted them.
We can discuss, which of my suggestions are mandatory for merging, and which not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have found the proper way to do things...
src/engine/Bind.cpp
Outdated
LocalVocab* outputLocalVocab, | ||
ad_utility::SimilarTo<IdTable> auto&& inputIdTable, | ||
const LocalVocab& inputLocalVocab, | ||
const sparqlExpression::SparqlExpression* expression, | ||
std::optional<std::pair<size_t, size_t>> subrange) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
Now that you've learned the whole truth about forwarding references (or more about it), we can do this the proper way:
This function needs its own IdTable, so it just takes IdTable
(by value) and no subrange
argument.
(And NO, you also don't really need the subrange for the evaluation context, if your input only consists of the relevant rows.
The you just have the idTable
which you use for both the evaluation context and for adding a column etc. to it.
The CALLS to this function then do the move / clone / cloneSubrange
which is then much safer etc. That way we also don't need the moveOrClone
function anymore .
Quality Gate passedIssues Measures |
Allow the
BIND
operation to handle its input lazily.NOTE: Currently there is only a single local vocab for all the results of the
BIND
, so even when aBIND
that creates strings is handled lazily, we still need the RAM for the complete local vocab. This will be handled in a follow-up PR.